-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/eng 382 unbond function #15
Conversation
Code coverage decreased because I added an untested |
Pull Request Test Coverage Report for Build 102
💛 - Coveralls |
Pull Request Test Coverage Report for Build 104
💛 - Coveralls |
// Only Bonded Delegators can call the function | ||
require(delegatorStatus(msg.sender) == DelegatorStatus.Bonded); | ||
// TODO: What if a Provider calls unbond() on himself ? | ||
// Should he resign ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do that next PR
require(delegatorStatus(msg.sender) == DelegatorStatus.Bonded); | ||
// TODO: What if a Provider calls unbond() on himself ? | ||
// Should he resign ? | ||
// What about the tokens of the Delegators that bonded to him ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refunded - but who pays the gas cost for that? Him, I suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but that could potentially be a huge cost in gas.
An alternative would be to let Delegators unbond() by themselves if they received the event that their Provider resigned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does Livepeer handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They resign the Transcoder without refunding the delegators.
test/TestTransmuteDPOS.spec.js
Outdated
const bondedAmount = (await tdpos.delegators(accounts[3]))[1].toNumber(); | ||
const newAmount = totalBondedAmount - bondedAmount; | ||
await tdpos.unbond({from: accounts[3]}); | ||
assert.equal(newAmount, (await tdpos.providers([accounts[0]]))[5].toNumber()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you annotate what the values associated with [0] and [5] are here?
|
||
it('should emit the DelegateUnbonded event', async () => { | ||
const delegator = await tdpos.delegators.call(accounts[5]); | ||
assert.equal(accounts[0], delegator[0]); // [0] is delegateAddress; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move these annotations to the first place that they are called in the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean ? Isn't it the first place they are called in this test ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better approach would be to use variables:
let delegateAddress = delegator[0]
let amountBonded = delegator[1]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes good idea
test/TestTransmuteDPOS.spec.js
Outdated
await approveBondProvider(22, 10, 1, 25, 1, accounts[0]); | ||
}); | ||
|
||
it('should return Unbonded if address in not a Delegator', async() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo - is*
test/TestTransmuteDPOS.spec.js
Outdated
assert.equal(DELEGATOR_UNBONDED, await tdpos.delegatorStatus(accounts[3])); | ||
}); | ||
|
||
it('should return Bonded if delegator has called bond()', async() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You capitalized 'Delegator' in all other tests' descriptions, let's keep that consistent.
https://transmute.atlassian.net/browse/ENG-382